Skip to content

Add support for specialization constants#2304

Open
ndgrigorian wants to merge 7 commits into
masterfrom
feature/specialization-constants
Open

Add support for specialization constants#2304
ndgrigorian wants to merge 7 commits into
masterfrom
feature/specialization-constants

Conversation

@ndgrigorian
Copy link
Copy Markdown
Collaborator

@ndgrigorian ndgrigorian commented Apr 28, 2026

This PR introduces support for specialization constants in dpctl, including both a Cython class SpecializationConstant for construction and passing of the constructed class to create_kernel_bundle_from_spirv via a new specializations keyword argument.

The SpecializationConstant class supports multiple constructors, including from Python buffers, a dtype string and a Python buffer (casting to the dtype via NumPy), and a number of bytes and a pointer as integers.

Also introduces dpctl.program.utils with parse_spirv_specializations utility function, allowing the user to query a SPIR-V directly from Python.

  • Have you provided a meaningful PR description?
  • Have you added a test, reproducer or referred to an issue with a reproducer?
  • Have you tested your changes locally for CPU and GPU devices?
  • Have you made sure that new changes do not introduce compiler warnings?
  • Have you checked performance impact of proposed changes?
  • Have you added documentation for your changes, if necessary?
  • Have you added your changes to the changelog?
  • If this PR is a work in progress, are you opening the PR as a draft?

@ndgrigorian ndgrigorian changed the title Feature/specialization constants Add support for specialization constants Apr 28, 2026
@github-actions
Copy link
Copy Markdown

@coveralls
Copy link
Copy Markdown
Collaborator

coveralls commented Apr 28, 2026

Coverage Status

coverage: 75.295% (-0.1%) from 75.39% — feature/specialization-constants into master

@ndgrigorian ndgrigorian force-pushed the feature/specialization-constants branch 3 times, most recently from b7f8d82 to 8c5651d Compare May 5, 2026 06:23
@ndgrigorian ndgrigorian marked this pull request as ready for review May 5, 2026 06:28
@ndgrigorian ndgrigorian force-pushed the feature/specialization-constants branch 3 times, most recently from 8dbb320 to 043aa83 Compare May 5, 2026 06:52
@ndgrigorian ndgrigorian force-pushed the rename-sycl-program-sycl-kernel-bundle branch from 82736b8 to 1a0a910 Compare May 7, 2026 01:12
@ndgrigorian ndgrigorian force-pushed the feature/specialization-constants branch from 55d4458 to 97871d9 Compare May 7, 2026 01:12
Base automatically changed from rename-sycl-program-sycl-kernel-bundle to master May 21, 2026 17:11
Comment thread dpctl/program/utils/__init__.py
spv_bytes: bytes | bytearray | memoryview,
) -> tuple[SpecializationConstantInfo]:
"""
Parses SPIR-V byte stream to extract information about specializations,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs to add rendering documentation for the new functions



class SpirvOpCode(IntEnum):
OpName = 5
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it makes sense to import the interesting constants directly from the SPIRV-Headers?
Or probably to add just a ref comment with a link to the code from where the constants come?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's no wheel package on PyPI for SPIRV-headers, which is why this wasn't done. In theory there is already spirv-dis which disassembles SPIRV and can be used to get some of the info, but for that same reason this wasn't done

A link makes sense in the ref though

}
}
else {
error_handler("clSetProgramSpecializationConstant is not available "
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing to release clProgram on error


if word_count == 0:
raise ValueError(f"Invalid SPIR-V instruction at word index {i}")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing boundary check:

if i + word_count > len(words):
    raise ValueError(f"Invalid SPIR-V: instruction at offset {i} extends beyond buffer")

)
elif isinstance(args[0], str):
target_obj = np.ascontiguousarray(args[1], dtype=args[0])

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need an explicit error handling here in case of else?

# attempt to coerce to a numpy array
target_obj = np.ascontiguousarray(target_obj)
else:
raise TypeError(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably it'd better to move raise TypeError(...) here from line 335, otherwise that branch is unreachable.

@@ -444,8 +480,22 @@ _CreateKernelBundleWithIL_ze_impl(const context &SyclCtx,
ZeDevice = get_native<ze_be>(SyclDev);

// Specialization constants are not supported by DPCTL at the moment
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the comment obsolete now?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, good catch


dtype_str = type_info["dtype"]
raw_default = defaults.get(target_id)
default_value = None
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems when raw_default is True or False (from OpSpecConstantTrue/OpSpecConstantFalse at lines 141, 146), the default value is not set in the result. Is that intended?

the second argument is interpreted as a pointer to the data.

Note that when constructing from a buffer, the
:class:`.SpecializationConstant`, shares memory with the original object.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably need to warn also about object lifetime: if the source object is deleted, the SpecializationConstant holds a dangling pointer.

also removes "v" as a permitted specialization constant intermediate data type, as composite specialization constants are broken into multiple specialization constants, so structs end up passed as a single constant while the program expects multiple, and therefore, doesn't work as intended
also adds spec_id, itemsize, and default_value fields
@ndgrigorian ndgrigorian force-pushed the feature/specialization-constants branch from 97871d9 to e2e4826 Compare May 24, 2026 01:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants